Skip to content

fix(whatsapp_data): recover from SQLite corruption instead of re-flooding Sentry (#4733)#4740

Merged
senamakel merged 8 commits into
tinyhumansai:mainfrom
oxoxDev:fix/4733-whatsapp-sqlite-corruption-recovery
Jul 13, 2026
Merged

fix(whatsapp_data): recover from SQLite corruption instead of re-flooding Sentry (#4733)#4740
senamakel merged 8 commits into
tinyhumansai:mainfrom
oxoxDev:fix/4733-whatsapp-sqlite-corruption-recovery

Conversation

@oxoxDev

@oxoxDev oxoxDev commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The whatsapp_data SQLite store now self-heals from an on-disk corruption (SQLITE_CORRUPT / "database disk image is malformed") instead of re-hitting a dead DB on every 2–30s scan tick.
  • On a confirmed malformed image the store quarantines the damaged file (preserved, never deleted), rebuilds an empty schema, and retries the write once — ingest resumes automatically.
  • Sentry is paged once per corruption episode via a process-wide latch (was: 1,813 escalating events from a single host — TAURI-RUST-KNH).
  • Adds a scoped expected-error classifier so any residual malformed-image envelope from the ingest RPC path is demoted (defense-in-depth, after the real recovery).

Problem

A user's whatsapp_data.db became corrupted (SQLITE_CORRUPT, code 11). Nothing detected or repaired it, so the whatsapp scanner's periodic ingest re-opened the malformed file and re-reported the same failure on every poll — 1,813 events, escalating, from one machine (Sentry TAURI-RUST-KNH). A malformed on-disk image never heals on its own: every write fails forever and the store had no recovery, no report latch, and the expected-error filter only recognised "database is locked" (busy), not corruption.

The codebase already had a proven recovery pattern for this exact failure in memory_queue / memory_store (corrupt-detect → quarantine → rebuild → integrity-check + a once-per-process report latch); whatsapp_data simply never adopted it.

Solution

Port the memory_store recovery pattern to whatsapp_data:

  • Detect (whatsapp_data/sqlite_retry.rs::is_sqlite_corrupt): match rusqlite ErrorCode::DatabaseCorrupt / NotADatabase (with a lowercased-text fallback for "disk image is malformed" / "file is not a database") through anyhow context layers.
  • Recover (whatsapp_data/store.rs::recover_corrupt_db): re-confirm with PRAGMA quick_check first (a transient mmap fault that now passes is not quarantined — good data is never destroyed), then rename the .db + -wal + -shm side-files to a timestamped .corrupt-<ts> copy (preserved for inspection/salvage), rebuild the schema via the existing init path, and PRAGMA integrity_check the result.
  • Bound it (write_with_corrupt_recovery): at most one recovery + one retry per write call, so a genuinely-wedged filesystem can't loop.
  • Report once (CORRUPT_REPORTED latch): first detection pages Sentry; the latch clears after a settled recovery so a genuinely-new, later corruption can still page exactly once.
  • Demote residual (core/observability.rs): new ExpectedErrorKind::WhatsAppDataSqliteCorrupt + a tightly-scoped classifier (requires the [whatsapp_data] ingest failed: envelope and an upsert wa_chat/wa_message frame and malformed-image text) so unrelated corruption elsewhere still reaches Sentry.

Wired into all three ingest writes (upsert_chats, upsert_messages, prune_old_messages) — the full write path is defended consistently. The store opens a fresh connection per call (no cached handle), so recovery needs no handle-drop before the rename; the next open picks up the rebuilt file. Recovery runs under the existing write-lock, so it stays serialized against concurrent writers.

RCA-vs-suppress: Bucket = real-defect (data path). The fix is the recovery; the classifier demotion is defense-in-depth after the store self-heals and pages once — never a substitute for the fix.

Submission Checklist

  • Tests added or updated (happy path + failure/edge case) — corrupt-DB recovery regression + healthy-DB no-op + detection matrix (codes + text + context layers + negative busy/constraint) + classifier positive/negative scoping
  • Diff coverage ≥ 80% — verified locally via cargo-llvm-cov + diff-cover --compare-branch=upstream/main: 91% on changed lines (234/256), store.rs 83.6%. Covered: corrupt→quarantine→rebuild→success, boot-time-init recovery, integrity-check-fails→Err (report-once preserved), prune-path classifier, detection matrix. Remaining uncovered lines are pure logging/error-context arms needing fault injection
  • Coverage matrix updated — N/A: behaviour-only reliability change to an existing store (no new feature row)
  • All affected feature IDs listed under ## RelatedN/A: no matrix feature IDs touched
  • No new external network dependencies introduced
  • Manual smoke checklist updated if release-cut surfaces touched — N/A: internal store reliability, no user-facing surface
  • Linked issue closed via Closes #NNN in ## Related

Impact

  • Platform: desktop (Windows/macOS/Linux) — WhatsApp ingest path.
  • Reliability: a corrupt whatsapp_data DB self-heals instead of wedging ingest permanently and flooding Sentry.
  • Data: damaged rows are not silently dropped — the corrupt image is preserved at .corrupt-<ts> for inspection/salvage; the rebuilt DB starts empty and re-populates from the next scan (WhatsApp data is a local cache re-derivable from the source).
  • Observability: one Sentry report per corruption episode instead of 1,813; residual RPC-path noise demoted.
  • No schema migration, no API change, no perf-sensitive path altered (recovery only runs on a confirmed corrupt image).

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

Commit & Branch

Validation Run

  • pnpm --filter openhuman-app format:check — N/A (no app/ changes)
  • pnpm typecheck — N/A (no TS changes)
  • Focused tests: recovery regression + detection + classifier tests pass in-tree (store_tests:: 39 passed incl. upsert_recovers_from_corrupt_database; sqlite_retry 9 passed; observability classifier passed)
  • Rust fmt/check (if changed): cargo fmt --check clean, cargo check --lib clean, cargo clippy --lib clean on all four changed files
  • Tauri fmt/check (if changed): N/A (no app/src-tauri changes)

Behavior Changes

  • Intended behavior change: a corrupt whatsapp_data SQLite DB is quarantined + rebuilt automatically instead of failing every ingest forever.
  • User-visible effect: WhatsApp history re-populates after a corruption instead of silently staying broken; no more Sentry flood.

Parity Contract

  • Legacy behavior preserved: healthy DBs are untouched (recovery only fires on a quick_check-confirmed corrupt image); busy/locked handling unchanged; no schema change.
  • Guard/fallback/dispatch parity checks: classifier is ordered before the busy classifier (corruption is more specific) and scoped so it cannot demote non-whatsapp or non-ingest corruption.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none
  • Canonical PR: this
  • Resolution: N/A

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

WhatsApp data writes and startup initialization now detect SQLite corruption, quarantine and rebuild damaged databases, validate recovered schemas, and control repeated reporting. Observability classifies matching corruption failures as expected warnings, with coverage for recovery and classification boundaries.

Changes

WhatsApp SQLite corruption recovery

Layer / File(s) Summary
SQLite corruption detection
src/openhuman/whatsapp_data/sqlite_retry.rs
Adds is_sqlite_corrupt using SQLite error codes and corruption text fallbacks, with wrapped-error and negative-case tests.
Store recovery and write integration
src/openhuman/whatsapp_data/store.rs, src/openhuman/whatsapp_data/store_tests.rs
Routes startup and writes through corruption-aware recovery, quarantines database files and side-files, rebuilds and validates the schema, and tests corrupted and healthy database behavior.
Expected-error reporting
src/core/observability.rs
Adds WhatsApp SQLite corruption classification, warning reporting, scoped matching, and classifier tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WhatsAppIngest
  participant WhatsAppDataStore
  participant SQLite
  participant Observability
  WhatsAppIngest->>WhatsAppDataStore: upsert chat or message
  WhatsAppDataStore->>SQLite: execute write with retry
  SQLite-->>WhatsAppDataStore: corruption error
  WhatsAppDataStore->>SQLite: quick_check and rebuild schema
  WhatsAppDataStore-->>WhatsAppIngest: recovered write result
  WhatsAppDataStore->>Observability: report corruption breadcrumb once
Loading

Poem

I’m a bunny with a database tune,
Quarantine shadows beneath the moon.
Fresh schemas bloom where bad bytes lay,
Warnings hop softly, then fade away.
Clean little writes can run again.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements corruption detection, quarantine/rebuild recovery, one-per-episode reporting, and scoped ingest error demotion as requested in #4733.
Out of Scope Changes check ✅ Passed The changes stay focused on WhatsApp SQLite corruption recovery and related tests, with no obvious unrelated functionality added.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: recovering WhatsApp data from SQLite corruption and reducing repeated Sentry reports.

Comment @coderabbitai help to get the list of available commands.

@oxoxDev oxoxDev force-pushed the fix/4733-whatsapp-sqlite-corruption-recovery branch from 250209d to b478652 Compare July 9, 2026 21:04
@oxoxDev oxoxDev marked this pull request as ready for review July 9, 2026 21:26
@oxoxDev oxoxDev requested a review from a team July 9, 2026 21:26

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b478652394

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +145 to +148
match retry_on_sqlite_busy(op_name, &f) {
Ok(val) => Ok(val),
Err(e) if is_sqlite_corrupt(&e) => {
self.report_and_recover(op_name, &e);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Recover corrupt DBs during store initialization

When whatsapp_data.db is already malformed at process startup, WhatsAppDataStore::new() calls init_schema() before any public write can enter this wrapper; that init error makes global::init leave the singleton unset, so subsequent ingest RPCs fail with “store accessed before init” instead of ever reaching the quarantine/rebuild path. This means a corruption that survives an app restart remains wedged and can still repeat on every scanner tick; handle is_sqlite_corrupt around startup schema init as well as write calls.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7bcfa2enew() now routes through init_schema_or_recover(): a boot-time is_sqlite_corrupt init failure drives quarantine+rebuild (reusing the report-once latch) then re-inits, so a corruption surviving restart self-heals instead of leaving the singleton unset. Test: new_recovers_from_corruption_at_startup.

Comment thread src/core/observability.rs Outdated
Comment on lines +832 to +833
if !(lower.contains("upsert wa_message") || lower.contains("upsert wa_chat")) {
return false;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Demote corrupt errors from the prune write path

This classifier only accepts upsert wa_chat/upsert wa_message frames, but the same commit wraps prune_old_messages in corrupt recovery. If corruption is detected from the prune path and recovery/retry still fails, the RPC error is shaped around prune old wa_messages (or the bare SQLite prepare/open error), so this predicate returns false and the residual RPC-path event can still page Sentry every scan tick; include the prune write context in the narrow match.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 60f6982 — the classifier now also accepts a prune frame (still gated by the [whatsapp_data] ingest failed: envelope + malformed-image text), and prune_old_messages_inner's bare prepare got a prune-marked .context(...). Tests: classifies_whatsapp_data_prune_path_sqlite_corrupt_errors + negative.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/openhuman/whatsapp_data/store.rs (2)

304-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

quick_check_ok and integrity_check_ok are nearly identical (one uses a raw Connection::open + inline busy_timeout, the other reuses open_conn/configure_connection). Could be collapsed into one helper parameterized by the pragma name, but the current duplication is small and the asymmetry (skipping WAL-mode pragma during the pre-quarantine check) may be intentional.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/whatsapp_data/store.rs` around lines 304 - 328,
`quick_check_ok` and `integrity_check_ok` duplicate nearly the same PRAGMA
execution flow in `store.rs`; factor the shared logic into a single helper that
takes the pragma name and returns the boolean result, then have both methods
delegate to it. Keep the existing behavior difference intact by preserving
`quick_check_ok`’s direct `Connection::open`/`busy_timeout` path versus
`integrity_check_ok`’s `open_conn`/`configure_connection` path if that
distinction is intentional, but centralize the common query-and-compare logic to
reduce duplication.

24-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Global latch is process-wide, not per-store-instance.

CORRUPT_REPORTED is a single process-wide static. If more than one WhatsAppDataStore is ever live in the same process (e.g. multiple workspaces/accounts with separate stores), corruption in one store's DB would suppress the Sentry report for an unrelated corruption episode in a different store. Currently harmless if only one store instance exists per process, but worth confirming that assumption holds.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/whatsapp_data/store.rs` around lines 24 - 31, The
CORRUPT_REPORTED latch in WhatsAppDataStore is process-wide, so a corruption in
one store can suppress Sentry reporting for a different store instance. Update
the dedupe state to be scoped per store (for example, inside WhatsAppDataStore
or keyed by the store identity used by the scanner logic) and adjust the
recovery/reset path accordingly. Use the CORRUPT_REPORTED symbol and the
surrounding WhatsAppDataStore/whatsapp_scanner corruption-handling flow to
locate and replace the global behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/whatsapp_data/store.rs`:
- Around line 286-302: The recovery path in `report_and_recover` ignores the
result of `integrity_check_ok()` because it always falls through to `Ok(true)`,
so the function reports success even when the rebuilt database is still
unhealthy or the integrity check errors. Update the match in
`report_and_recover` to return the integrity-check outcome explicitly: keep the
success branch as `Ok(true)`, and make the `Ok(false)` and `Err(e)` branches
return `Err(...)` (or otherwise propagate failure) after logging. This keeps the
`CORRUPT_REPORTED` reset and “ingest will resume” behavior in
`report_and_recover` aligned with actual recovery success.
- Around line 256-278: The quarantine loop in `recover_corrupt_db()` is too
strict for `-wal`/`-shm` side-files and can abort before `init_schema()` runs,
which leaves a recreated empty DB behind. Update the `whatsapp_data` quarantine
logic to make side-file moves best-effort after the main database file is
quarantined, or ensure the schema is rebuilt even if a side-file rename fails.
Keep the main file recovery path intact and adjust the
`with_name_suffix`/`std::fs::rename` handling so failures on `-wal` or `-shm` do
not stop the recovery flow.

---

Nitpick comments:
In `@src/openhuman/whatsapp_data/store.rs`:
- Around line 304-328: `quick_check_ok` and `integrity_check_ok` duplicate
nearly the same PRAGMA execution flow in `store.rs`; factor the shared logic
into a single helper that takes the pragma name and returns the boolean result,
then have both methods delegate to it. Keep the existing behavior difference
intact by preserving `quick_check_ok`’s direct `Connection::open`/`busy_timeout`
path versus `integrity_check_ok`’s `open_conn`/`configure_connection` path if
that distinction is intentional, but centralize the common query-and-compare
logic to reduce duplication.
- Around line 24-31: The CORRUPT_REPORTED latch in WhatsAppDataStore is
process-wide, so a corruption in one store can suppress Sentry reporting for a
different store instance. Update the dedupe state to be scoped per store (for
example, inside WhatsAppDataStore or keyed by the store identity used by the
scanner logic) and adjust the recovery/reset path accordingly. Use the
CORRUPT_REPORTED symbol and the surrounding WhatsAppDataStore/whatsapp_scanner
corruption-handling flow to locate and replace the global behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3565bfc0-c4ef-4dda-a458-07a5946def2e

📥 Commits

Reviewing files that changed from the base of the PR and between 54511e6 and b478652.

📒 Files selected for processing (4)
  • src/core/observability.rs
  • src/openhuman/whatsapp_data/sqlite_retry.rs
  • src/openhuman/whatsapp_data/store.rs
  • src/openhuman/whatsapp_data/store_tests.rs

Comment thread src/openhuman/whatsapp_data/store.rs
Comment thread src/openhuman/whatsapp_data/store.rs Outdated
oxoxDev added 8 commits July 13, 2026 11:22
…sai#4733)

Add is_sqlite_corrupt to recognise a malformed on-disk image
(SQLITE_CORRUPT code 11 / SQLITE_NOTADB code 26), by rusqlite error code
with a flattened-string text fallback. This is the signal the store uses
to trigger quarantine + rebuild recovery instead of retrying a dead file
on every 2-30s scan tick (Sentry TAURI-RUST-KNH: 1,813 escalating events
from one host).
…umansai#4733)

On a confirmed SQLITE_CORRUPT malformed image, the store now quarantines
the damaged whatsapp_data.db (+ WAL/SHM side-files) to a timestamped
.corrupt-<ts> copy (preserved, never deleted), rebuilds an empty schema
via init_schema, and confirms with PRAGMA integrity_check — mirroring the
memory_store/memory_queue pattern. quick_check pre-confirms corruption so
a transient mmap fault never destroys good data.

The two upsert paths and prune route through write_with_corrupt_recovery,
which drives recovery at most once per call then retries once against the
rebuilt DB, so ingest self-heals instead of wedging. A process-wide
CORRUPT_REPORTED latch reports to Sentry once per corruption episode
(reset after a successful recovery) instead of on every scan tick.
…ed (tinyhumansai#4733)

Add is_whatsapp_data_sqlite_corrupt_message + a WhatsAppDataSqliteCorrupt
kind that demotes '[whatsapp_data] ingest failed: ... database disk image
is malformed / file is not a database' out of the Sentry error stream.

Defense-in-depth AFTER the store's quarantine+rebuild recovery, not
instead of it: it only covers residual noise in the window between
detection and a successful rebuild (or a rebuild that keeps failing on a
wedged host FS). Scoped to the whatsapp ingest+upsert envelope so
unrelated malformed-image errors in other domains still reach Sentry.
upsert_recovers_from_corrupt_database writes a malformed image at the
whatsapp_data.db path, runs the upsert path, and asserts recovery
quarantines the bad file (preserved, not deleted), rebuilds, and the same
upsert SUCCEEDS against the fresh DB — with the report latch reset after
recovery (fires at most once per episode). recover_corrupt_db_is_noop_on_healthy_db
asserts a healthy DB is never quarantined and its data survives.
@oxoxDev oxoxDev force-pushed the fix/4733-whatsapp-sqlite-corruption-recovery branch from b478652 to 11877ea Compare July 13, 2026 06:27

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/openhuman/whatsapp_data/store_tests.rs (2)

619-678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Boot-time recovery test doesn't verify the report latch resets.

upsert_recovers_from_corrupt_database explicitly asserts the latch resets after a successful write-path recovery (lines 550-553), but new_recovers_from_corruption_at_startup only unconditionally resets CORRUPT_REPORTED at the end (line 677) without first asserting its value. Since init_schema_or_recover drives recovery through a distinct call path (report_and_recover("init_schema", ...)), this leaves the latch-reset contract unverified for the boot path specifically.

✅ Proposed addition
     assert_eq!(rows.len(), 1);
     assert_eq!(rows[0].chat_id, "chat@c.us");

+    assert!(
+        !super::CORRUPT_REPORTED.load(Ordering::Relaxed),
+        "report latch must reset after a successful boot-time recovery"
+    );
     super::CORRUPT_REPORTED.store(false, Ordering::Relaxed);
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/whatsapp_data/store_tests.rs` around lines 619 - 678, Extend
new_recovers_from_corruption_at_startup to assert that CORRUPT_REPORTED is
cleared after the successful upsert, before its existing unconditional reset.
Keep the assertion focused on the boot-time recovery path and preserve the
current cleanup reset afterward.

465-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated corruption-injection logic across tests.

The "remove WAL/SHM side-files, then overwrite the main DB with garbage bytes" sequence (lines 490-498) is repeated almost verbatim as a local closure in recover_corrupt_db_errors_and_keeps_latch_when_rebuild_fails_integrity (lines 576-582), and a third time (without the WAL/SHM removal step, since the workspace is fresh) in new_recovers_from_corruption_at_startup (lines 634-638). Consider hoisting a shared fn corrupt_db_file(path: &Path) helper near make_store/db_path_for to avoid drift between the copies.

♻️ Proposed helper extraction
+fn corrupt_db_file(db_path: &std::path::Path) {
+    for suffix in ["-wal", "-shm"] {
+        let side = db_path.with_file_name(format!("whatsapp_data.db{suffix}"));
+        let _ = std::fs::remove_file(&side);
+    }
+    std::fs::write(db_path, b"this is not a sqlite database, just garbage bytes").unwrap();
+}

Then replace each inline block/closure with corrupt_db_file(&db_path);.

Also applies to: 556-561, 562-582

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/whatsapp_data/store_tests.rs` around lines 465 - 498, Extract
the repeated database-corruption setup into a shared corrupt_db_file helper near
make_store/db_path_for, accepting a Path reference, removing the -wal and -shm
side files, and overwriting the main database with the existing garbage bytes.
Replace the inline corruption blocks in upsert_recovers_from_corrupt_database,
recover_corrupt_db_errors_and_keeps_latch_when_rebuild_fails_integrity, and
new_recovers_from_corruption_at_startup with calls to this helper, preserving
each test’s existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/openhuman/whatsapp_data/store_tests.rs`:
- Around line 684-712: Acquire super::CORRUPT_TEST_GUARD at the start of
recover_corrupt_db_is_noop_on_healthy_db, before calling
store.recover_corrupt_db(), matching the sibling corruption tests and
serializing access to the process-wide corruption-test statics.

---

Nitpick comments:
In `@src/openhuman/whatsapp_data/store_tests.rs`:
- Around line 619-678: Extend new_recovers_from_corruption_at_startup to assert
that CORRUPT_REPORTED is cleared after the successful upsert, before its
existing unconditional reset. Keep the assertion focused on the boot-time
recovery path and preserve the current cleanup reset afterward.
- Around line 465-498: Extract the repeated database-corruption setup into a
shared corrupt_db_file helper near make_store/db_path_for, accepting a Path
reference, removing the -wal and -shm side files, and overwriting the main
database with the existing garbage bytes. Replace the inline corruption blocks
in upsert_recovers_from_corrupt_database,
recover_corrupt_db_errors_and_keeps_latch_when_rebuild_fails_integrity, and
new_recovers_from_corruption_at_startup with calls to this helper, preserving
each test’s existing behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb292f08-7cac-45c7-9500-620c3b60ab26

📥 Commits

Reviewing files that changed from the base of the PR and between b478652 and 11877ea.

📒 Files selected for processing (4)
  • src/core/observability.rs
  • src/openhuman/whatsapp_data/sqlite_retry.rs
  • src/openhuman/whatsapp_data/store.rs
  • src/openhuman/whatsapp_data/store_tests.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/openhuman/whatsapp_data/sqlite_retry.rs
  • src/openhuman/whatsapp_data/store.rs

Comment on lines +684 to +712
fn recover_corrupt_db_is_noop_on_healthy_db() {
let (store, tmp) = make_store();
let db_path = db_path_for(&tmp);
// Seed a real row so there is genuine data that must survive.
let mut chats = HashMap::new();
chats.insert("chat@c.us".to_string(), chat_meta("Alice"));
store.upsert_chats("acct1", &chats).unwrap();

let recovered = store
.recover_corrupt_db()
.expect("recovery on a healthy DB must not error");
assert!(!recovered, "healthy DB must not be quarantined");

let quarantined = std::fs::read_dir(db_path.parent().unwrap())
.unwrap()
.filter_map(|e| e.ok())
.any(|e| e.file_name().to_string_lossy().contains(".corrupt-"));
assert!(!quarantined, "no quarantine file should be created");

// Data survives untouched.
let rows = store
.list_chats(&ListChatsRequest {
account_id: Some("acct1".to_string()),
limit: None,
offset: None,
})
.unwrap();
assert_eq!(rows.len(), 1);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Missing CORRUPT_TEST_GUARD lock — inconsistent with sibling tests touching the same statics.

The other three new corruption tests all acquire super::CORRUPT_TEST_GUARD before touching CORRUPT_REPORTED / FORCE_INTEGRITY_CHECK_FAIL, explicitly to serialize access to these process-wide statics under Rust's default parallel test execution. This test calls store.recover_corrupt_db() directly without the guard. It's currently safe only because the healthy-DB quick_check fast path (per the upstream recover_corrupt_db contract) returns Ok(false) before touching either static — but that's an implicit invariant this test doesn't defend, and a future change to the fast path (or to recover_corrupt_db's early logic) could silently introduce cross-test flakiness.

🔒 Proposed fix
 #[test]
 fn recover_corrupt_db_is_noop_on_healthy_db() {
+    let _guard = super::CORRUPT_TEST_GUARD
+        .lock()
+        .unwrap_or_else(|e| e.into_inner());
     let (store, tmp) = make_store();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn recover_corrupt_db_is_noop_on_healthy_db() {
let (store, tmp) = make_store();
let db_path = db_path_for(&tmp);
// Seed a real row so there is genuine data that must survive.
let mut chats = HashMap::new();
chats.insert("chat@c.us".to_string(), chat_meta("Alice"));
store.upsert_chats("acct1", &chats).unwrap();
let recovered = store
.recover_corrupt_db()
.expect("recovery on a healthy DB must not error");
assert!(!recovered, "healthy DB must not be quarantined");
let quarantined = std::fs::read_dir(db_path.parent().unwrap())
.unwrap()
.filter_map(|e| e.ok())
.any(|e| e.file_name().to_string_lossy().contains(".corrupt-"));
assert!(!quarantined, "no quarantine file should be created");
// Data survives untouched.
let rows = store
.list_chats(&ListChatsRequest {
account_id: Some("acct1".to_string()),
limit: None,
offset: None,
})
.unwrap();
assert_eq!(rows.len(), 1);
}
fn recover_corrupt_db_is_noop_on_healthy_db() {
let _guard = super::CORRUPT_TEST_GUARD
.lock()
.unwrap_or_else(|e| e.into_inner());
let (store, tmp) = make_store();
let db_path = db_path_for(&tmp);
// Seed a real row so there is genuine data that must survive.
let mut chats = HashMap::new();
chats.insert("chat@c.us".to_string(), chat_meta("Alice"));
store.upsert_chats("acct1", &chats).unwrap();
let recovered = store
.recover_corrupt_db()
.expect("recovery on a healthy DB must not error");
assert!(!recovered, "healthy DB must not be quarantined");
let quarantined = std::fs::read_dir(db_path.parent().unwrap())
.unwrap()
.filter_map(|e| e.ok())
.any(|e| e.file_name().to_string_lossy().contains(".corrupt-"));
assert!(!quarantined, "no quarantine file should be created");
// Data survives untouched.
let rows = store
.list_chats(&ListChatsRequest {
account_id: Some("acct1".to_string()),
limit: None,
offset: None,
})
.unwrap();
assert_eq!(rows.len(), 1);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/openhuman/whatsapp_data/store_tests.rs` around lines 684 - 712, Acquire
super::CORRUPT_TEST_GUARD at the start of
recover_corrupt_db_is_noop_on_healthy_db, before calling
store.recover_corrupt_db(), matching the sibling corruption tests and
serializing access to the process-wide corruption-test statics.

@senamakel senamakel merged commit aae842e into tinyhumansai:main Jul 13, 2026
20 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

whatsapp_data: recover from SQLite corruption (disk image malformed) instead of re-reporting every poll

2 participants